feat(mfa): add IRecoveryCodeService::disableTwoFactor by romanetar · Pull Request #169 · OpenStackweb/openstackid

romanetar · 2026-10-07T17:42:40Z

ref: https://app.clickup.com/t/86ba2zp7r

Stacked on #166 (base: feat/mfa-admin-enforcement-and-reset-commands), which is stacked on #125. Merge bottom-up: #125, #166, then this one.

Summary

Provisional implementation of the shared disable/reset service method that ClickUp ticket 12 (idp:reset-2fa) needs and ticket 14 (profile toggle and admin CRUD) will reuse. The ticket allows implementing it in ticket 12 when ticket 14 has not landed, with this exact contract:

IRecoveryCodeService::disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void

In a single transaction: disable2FA() (clears two_factor_enabled and two_factor_enforced_at), deletes every recovery code, and revokes every trusted device (is_revoked = 1, rows are kept).
Afterwards records a device_revoked audit event, best-effort: a failure there does not fail the operation.
$actor decides the password rule:
- null (console, e.g. idp:reset-2fa): no password.
- another user (an administrator): no password. Authorizing the administrator is the caller's job.
- the same user (self-service): $currentPassword must match, otherwise ValidationException and nothing changes.
It does not emit settings_changed: the caller knows the context (reason, actor, operator) and emits it, so there is exactly one row.
Group-enforced users are still challenged at login afterwards, since enforcement is derived from group membership (User::shouldRequire2FA()).

Things to know

It does not call DeviceTrustService::removeTrustedDevices as the ticket suggests: that method's audit call opens its own transaction, and nesting transactions can close the entity manager if the inner one fails (the existing code in RecoveryCodeService warns about this). It uses IUserTrustedDeviceRepository::revokeAllForUser inside the same transaction instead, which is what removeTrustedDevices does for the DB part.
RecoveryCodeService gets a new constructor dependency (IUserTrustedDeviceRepository). Nothing instantiates it with new; the container resolves it.
If ticket 14 lands its own disableTwoFactor, the two need to be reconciled; ticket 14 can adopt this one directly.

Test plan

tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php: 6 tests (console, admin, self-service with correct / wrong / missing password, audit failure).
tests/DisableTwoFactorIntegrationTest.php: 2 tests against the DB (everything cleared, 2 devices revoked, 1 device_revoked row and no settings_changed; wrong password leaves state untouched).
Full phpunit suite not run. tests/RecoveryCodeRegenerationTest.php already fails on the base branch (Class "Strategies\MFA\MFAChallengeStrategyFactory" not found), unrelated to this change.

coderabbitai · 2026-10-07T17:42:50Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 372bdafd-9c86-45bd-8f79-239062e96bc4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

Trigger review

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

github-actions · 2026-10-07T17:44:05Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

github-actions · 2026-11-04T13:22:48Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

github-actions · 2026-11-04T13:22:52Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

Copilot

🟢 Approval recommended

No active review issues remain for the shared two-factor disabling service.

No open issues 🎉

What changed in this PR

Adds a shared disableTwoFactor service method for the planned MFA reset command and profile/admin controls, building on the existing MFA infrastructure.

Changes:

Defines the actor-based password requirement and caller-owned settings_changed audit contract.
Disables 2FA, deletes recovery codes, and revokes trusted devices in one transaction, followed by best-effort audit logging.
Adds unit and database integration tests covering actor rules, state cleanup, and audit failure handling.

File	Description
`tests/unit/RecoveryCodeServiceDisableTwoFactorTest.php`	Tests password rules, disable operations, and audit failure handling.
`tests/DisableTwoFactorIntegrationTest.php`	Verifies database cleanup, audit events, and unchanged state after an incorrect password.
`app/Services/Auth/RecoveryCodeService.php`	Implements transactional disabling and trusted-device revocation.
`app/Services/Auth/IRecoveryCodeService.php`	Declares the method and documents password, audit, and group-enforcement behavior.

_{Review effort: Balanced}

smarcet · 2026-11-04T17:28:31Z

+        // a single transaction() call: nesting another one (e.g. through the audit
+        // service) inside it would let an inner failure close the entity manager
+        // under this still-running outer transaction
+        $this->tx_service->transaction(function () use ($user) {


@romanetar Once the users-row lock from SDS rev 2.6 lands (https://github.com/fntechgit/ftn-docsnsklz/pull/179, §3.5; ticket https://app.clickup.com/t/86bcy7rv7, task 3), this transaction should take the same PESSIMISTIC_WRITE lock on the user's row first. It deletes recovery codes too, and without the lock an admin reset racing a user regenerate (READ_COMMITTED, bulk DELETE with no lock) can let the regenerate insert after the delete, leaving a "reset" user with a fresh batch.

No change needed in this PR: the lock does not exist yet. It is tracked in task 9 of the recovery-code enrollment ticket, next to resetting recovery_codes_acknowledged_at here.

Agreed, no change here. Noted that when the users-row lock lands, generateRecoveryCodes and enableTwoFactorAndGenerateCodes need it too, not just disableTwoFactor; otherwise the race remains. Tracked in task 9.

github-actions · 2026-11-04T17:44:13Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

github-actions · 2026-11-04T17:44:13Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

Shared service method that disables 2FA for a user: clears two_factor_enabled and two_factor_enforced_at, deletes every recovery code and revokes every trusted device in a single transaction, then records a device_revoked audit event best-effort. The actor decides the password rule: null (console) and another user (admin) need no password, the user acting on their own account must provide the current one. The settings_changed audit event is left to the caller, which knows the context (reason, actor, operator). Group-enforced users are still challenged at login afterwards, as enforcement is derived from group membership.

github-actions · 2026-11-10T16:15:16Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

A social-only user cannot pass the self-service password check, as User::checkPassword() returns false for an empty password. Document that the caller must require another proof of identity (a recent 2FA challenge) and pin the behaviour with a unit test.

github-actions · 2026-11-10T16:44:44Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

…d per device Replaces the revokeAllForUser bulk UPDATE, which the trusted-devices branch removes. disableTwoFactor and removeTrustedDevices revoke inside their transaction and log one device_revoked (with device_id) per revoked device after commit, with the same semantics as the devices-management endpoints. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

github-actions · 2026-11-10T18:16:53Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

…eTwoFactorIntegrationTest

github-actions · 2026-11-10T19:05:43Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-169/

This page is automatically updated on each push to this PR.

smarcet · 2026-11-13T18:48:40Z

+    /**
+     * @inheritDoc
+     */
+    public function disableTwoFactor(User $user, ?string $currentPassword, ?User $actor): void


@romanetar #186 adds a second RecoveryCodeService::disableTwoFactor() with the same signature, and git merges this file without a conflict: only IRecoveryCodeService.php and the two test files conflict. git merge-tree of this branch and feat/mfa-disable-two-factor-self-service leaves both methods in RecoveryCodeService.php (lines 238 and 300 of the merged file), and php -l fails with Cannot redeclare App\Services\Auth\RecoveryCodeService::disableTwoFactor(). The container resolves this class in the UserController and UserApiController constructors, so a tree merged that way breaks those routes, login included.

The two versions also differ in behaviour:

feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 rejects non-console calls for group-enforced users; this one has no group guard.

feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 takes lockUserRow() and clears recovery_codes_acknowledged_at; this one does neither.

feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 still calls revokeAllForUser(), which this PR and #160 remove. Merging feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 with feat(2fa): trusted devices management UI and revoke endpoints #160 leaves that call in place (RecoveryCodeService.php:275).

feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186 writes an enrollment_changed row inside the service, while this contract leaves the event to the caller and idp:reset-2fa (#171) writes settings_changed. A reset would end up writing both.

Suggested fix: keep one method before either PR merges. This one is what #171 consumes and what SDS rev 2.6 describes (per-device device_revoked, fntechgit/ftn-docsnsklz#179). Port #186's group guard, row lock, acknowledgment reset and passwordless window into it instead of adding a second definition, and decide whether the enrollment event comes from the service or the caller. Then run php -l and phpunit on the combined tree.

smarcet

@romanetar please review

romanetar added this pull request to stack #170 October 7, 2026 17:46

romanetar mentioned this pull request Oct 7, 2026

feat(mfa): add idp:reset-2fa command #171

Open

3 of 4 tasks

smarcet force-pushed the feat/mfa-disable-two-factor-service branch from bd66c1f to 3df2d8e Compare November 4, 2026 13:21

smarcet assigned romanetar Nov 4, 2026

smarcet requested review from smarcet and a balanced review from Copilot November 4, 2026 13:23

Copilot started reviewing on behalf of smarcet November 4, 2026 13:24

View session

Copilot AI reviewed Nov 4, 2026

View reviewed changes

smarcet reviewed Nov 4, 2026

View reviewed changes

Comment thread app/Services/Auth/RecoveryCodeService.php Resolved

smarcet reviewed Nov 4, 2026

View reviewed changes

smarcet force-pushed the feat/mfa-disable-two-factor-service branch from 3df2d8e to fd29e5c Compare November 4, 2026 17:42

smarcet mentioned this pull request Nov 4, 2026

feat(2fa): trusted devices management UI and revoke endpoints #160

Merged

smarcet reviewed Nov 4, 2026

View reviewed changes

Comment thread app/Services/Auth/RecoveryCodeService.php Resolved

This was referenced Nov 6, 2026

feat(2fa): recovery-code enrollment modal and batch acknowledgment #183

Open

feat(2fa): disableTwoFactor service method with enrollment_changed audit and group guard #186

Open

romanetar force-pushed the feat/mfa-disable-two-factor-service branch from fd29e5c to c3f0def Compare November 10, 2026 16:11

test(mfa): expect one device_revoked audit event per device in Disabl…

94e505c

…eTwoFactorIntegrationTest

smarcet reviewed Nov 13, 2026

View reviewed changes

smarcet requested changes Nov 13, 2026

View reviewed changes

Conversation

romanetar commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Things to know

Test plan

Uh oh!

coderabbitai Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Oct 7, 2026

Uh oh!

github-actions Bot commented Nov 4, 2026

Uh oh!

github-actions Bot commented Nov 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

smarcet Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

romanetar Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 4, 2026

Uh oh!

github-actions Bot commented Nov 4, 2026

Uh oh!

Uh oh!

github-actions Bot commented Nov 10, 2026

Uh oh!

github-actions Bot commented Nov 10, 2026

Uh oh!

github-actions Bot commented Nov 10, 2026

Uh oh!

github-actions Bot commented Nov 10, 2026

Uh oh!

smarcet Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

smarcet left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

romanetar commented Oct 7, 2026 •

edited

Loading

coderabbitai Bot commented Oct 7, 2026 •

edited

Loading